Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copter: Set the message buffer size to twice the message size #28211

Conversation

muramura
Copy link
Contributor

The number of characters in the message exceeds 50 bytes.
The message array is currently set to 50 bytes. I will submit a PR to change the array to 100 bytes.
ArduPilot has string splitting functionality.

AFTER
Screenshot from 2024-09-24 11-26-39

BEFORE
Screenshot from 2024-09-24 09-58-46

THIS SOURCE:
Screenshot from 2024-09-24 09-59-20

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

The message you point out has bugged me for a while!

@rmackay9
Copy link
Contributor

@peterbarker I guess we don't care about an extra 50 bytes of RAM being used? The case where the message is too long, it's only long by a few bytes I think so we could just make it 60 instead of 100.

take care of the pennies and the pounds will take care of themselves

@muramura
Copy link
Contributor Author

muramura commented Sep 26, 2024

@rmackay9 san.
The variable that stores the text is an automatic variable.
Therefore, this area is temporary.
The stack and flash memory are different.

ex:
libraries/AP_NMEA_Output/AP_NMEA_Output.cpp
The AP_NMEA_Output::update method in this file requires a large amount of stack. This method temporarily consumes over 300 bytes of stack.

Screenshot from 2024-09-27 06-54-42

@rmackay9
Copy link
Contributor

ok, thanks. I think there must be a cost to having the array larger than necessary

@peterbarker if you want to go ahead and merge this then I'm OK with that.

@peterbarker
Copy link
Contributor

ok, thanks. I think there must be a cost to having the array larger than necessary

Not really - these are stack allocations, so they're only temporary buffers. Our stack from the main thread is fine for this sort of thing.

The main concern is people going overboard with longer messages now! I've stared at this particular message a few times and failed to find a way to make it shorter and as understandable.

Merging this one.

@peterbarker peterbarker merged commit 13a5dc8 into ArduPilot:master Sep 27, 2024
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants